🔍 Rust PR Review

Summary: Solid structural refactor with one functional bug and two minor issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs + standalone.rs: The project field in AzureDevOpsOptions is exposed in the public API, documented in AGENTS.md (overridable via org: field), and tested in types.rs — but ado_config.project() is never called anywhere in the compilation pipeline. Users who set azure-devops: project: MyProject will see no effect; the value is silently discarded. Either wire it into the entrypoint args (probably a -p <project> flag if @azure-devops/mcp supports that) or remove the field and tests before shipping.

🔒 Security Concerns

• templates/base.yml:257: The $(ADO_ORG_NAME) placeholder in the MCPG JSON config uses ADO variable expansion syntax, while all other in-config placeholders (${SAFE_OUTPUTS_PORT}, etc.) intentionally use bash ${} syntax to avoid ADO expansion. This means the sed escape relies on ADO leaving undefined variables unexpanded (\$(ADO_ORG_NAME) → bash literal $(ADO_ORG_NAME)). ADO's behavior for undefined variables is not guaranteed — the $$ form is the documented escape. If ADO does expand the undefined $(ADO_ORG_NAME) to empty string, the sed command silently fails and the ADO MCP starts with an empty org name. Recommend using ${ADO_ORG_NAME} as the placeholder in the generated JSON to stay consistent with the other substitutions and avoid the ADO-vs-bash ambiguity.

⚠️ Suggestions

• src/compile/common.rs:629: MCPG_IMAGE is defined and documented in AGENTS.md as the {{ mcpg_image }} template marker, but standalone.rs handles no such marker, and base.yml still has ghcr.io/github/gh-aw-mcpg hardcoded directly. The constant is dead code. Either wire up the {{ mcpg_image }} replacement in standalone.rs (using MCPG_IMAGE) so the image name is controlled from one place, or remove the constant and the documentation entry.

• src/mcp.rs:1364: Stale comment says "non-MCP keys like 'memory'" — memory was removed from NON_MCP_SAFE_OUTPUT_KEYS (now &[]) in this very PR. The comment should be updated or removed.

✅ What Looks Good

• The src/tools/ → src/safeoutputs/ rename is a clean semantic split — the new names make the two-phase architecture (Stage 1 NDJSON serialization vs. first-class tool integration) much clearer.
• The #[serde(untagged)] bool/object enum pattern for CacheMemoryToolConfig and AzureDevOpsToolConfig is consistent with ScheduleConfig / McpConfig — no surprises.
• Single-quoted heredoc (<< 'MCPG_CONFIG_EOF') correctly prevents both bash and ADO expansion when writing the MCPG config, which is exactly right.
• Token passthrough for the ADO MCP is correctly gated on permissions.read — won't silently fail for pipelines without a read service connection.
• Warning on tools.azure-devops + mcp-servers.azure-devops double-configuration is good UX.
• Test coverage for the new MCPG config generation is thorough (8 new integration tests + 9 type deserialization tests).

Generated by Rust PR Reviewer for issue #167 · ● 5.6M · ◷

🔍 Rust PR Review

Summary: Looks good overall — clean, well-structured refactor with solid test coverage. Two suggestions worth addressing before merge.

Findings

⚠️ Suggestions

• src/compile/standalone.rs:555-576 — org and toolset values are not validated before insertion into MCPG entrypoint args

  User-supplied tools.azure-devops.org: and every entry in toolsets: are inserted directly into entrypointArgs with no character validation:

  entrypoint_args.push(org);             // from tools.azure-devops.org
  for toolset in ado_config.toolsets() {
      entrypoint_args.push(toolset.clone()); // user-controlled
  }

  JSON serialization prevents shell injection (args are passed as an exec array, not a shell string), but invalid values (e.g. an org name containing spaces or slashes) will produce a cryptic MCPG runtime failure instead of a clear compile-time error. The existing is_safe_tool_name helper in common.rs (ASCII alphanumerics + hyphens) would work as a validator for both:

  if !org.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') {
      anyhow::bail!("Invalid ADO org name '{org}': must contain only alphanumerics and hyphens");
  }

  Toolset names (repos, wit, core, etc.) are also worth validating with the same rule, consistent with how safe-outputs tool names are validated. The allowed: list (goes into MCPG's tools field, not into shell args) is lower priority but symmetric.

• src/compile/common.rs:852-855 — Generic warning for safe-outputs: memory: will silently break existing users

  After this PR, front matter that still contains safe-outputs: memory: hits the generic "unrecognized safe-output tool 'memory' — skipping" path with no actionable guidance. Users who upgrade without reading the changelog will see the warning but won't know why their memory feature stopped working.

  A specific deprecation hint would prevent confusion:

  if key == "memory" {
      eprintln!(
          "Warning: Agent '{}': 'safe-outputs: memory:' has moved to \
           'tools: cache-memory:'. Update your front matter to restore memory support.",
          front_matter.name
      );
      continue;
  }

  This doesn't affect correctness, but the breakage is otherwise silent beyond the generic warning.

✅ What Looks Good

• Module restructure — safeoutputs/ vs tools/ distinction is clean and well-motivated. The mod.rs re-exports and import updates are thorough.
• generate_mcpg_config → Result<McpgConfig> — the function signature change is correct; all existing call sites were updated with .unwrap() in tests (appropriate) and ? in production paths.
• {{ mcpg_image }} parameterization — good improvement; decouples image name from the version field and enables independent override later.
• Precedence logic for org resolution (explicit > inferred > compile error) is clear, well-documented, and fully tested, including the no-org-fails case (test_ado_tool_no_org_fails).
• permissions.read absence warning — generate_mcpg_docker_env correctly emits a warning when tools.azure-devops is enabled but no read service connection is configured. The token-forwarding logic (ado_tool_needs_token) is correct.
• tools.azure-devops: false is tested to produce no MCPG entry — important edge case.
• Test coverage — 8 new ADO MCPG tests + 9 deserialization tests cover the main paths well.

Generated by Rust PR Reviewer for issue #167 · ● 2.4M · ◷

🔍 Rust PR Review

Summary: Solid refactor — well-structured, correct security controls, clean module separation. One missing compile-time guard and a PR description inaccuracy worth noting.

Findings

⚠️ Suggestions

• src/compile/standalone.rs (generate_mcpg_config): No compile-time warning when tools: azure-devops: is enabled but permissions.read is not configured. The ADO MCP unconditionally injects AZURE_DEVOPS_EXT_PAT: "" (passthrough), but that passthrough only gets wired into the MCPG Docker env when permissions.read is set (via generate_mcpg_docker_env). Without it, the MCP container starts with an empty PAT and every ADO API call will 401 silently. A validate_write_permissions-style check (or at least an eprintln! warning) at compile time would surface this before runtime.

• src/compile/standalone.rs (generate_mcpg_config): The allowed list items from tools.azure-devops.allowed (e.g. wit_get_work_item) are placed directly into the MCPG JSON config without validation. These go into JSON (not a shell command), so there's no injection risk — but they can contain arbitrary Unicode or shell-special chars that would produce an invalid MCPG config. The existing is_safe_tool_name function in common.rs already validates alphanumeric+hyphens; applying it here would be consistent with how safe-outputs tool names are validated. Toolsets and org names are already validated; this one was missed.

• PR description accuracy: The description says org is "auto-inferred from $(System.TeamFoundationCollectionUri) at runtime" — but the code infers it from the git remote URL at compile time (via get_git_remote_url + parse_ado_remote). This could mislead users who move the compiled pipeline to a different org; the org is baked into the generated mcpg-config.json at compile time, not resolved dynamically.

🐛 Bugs / Logic Issues

• src/compile/common.rs (generate_enabled_tools_args): The memory key check now emits a warning and continues, but then falls through to the effective_mcp_tool_count == 0 early-return path — which returns String::new() (all tools allowed). If someone has safe-outputs: memory: plus a legitimate tool like create-pull-request, the continue means create-pull-request is still counted correctly and the --enabled-tools list is correct. ✅ This is fine; just confirming the logic is sound.

• src/main.rs execute path: Old configs with safe-outputs: memory: will silently get no memory processing at stage 2 (only a compile-time eprintln! warning). The PR marks this as "No backward compatibility — clean break", so this is intentional, but worth verifying pipeline operators are aware that recompilation is required.

✅ What Looks Good

• Security: Org names and toolset names are validated (alphanumeric + hyphens only) before being pushed into the entrypoint_args Vec. Even though those args ultimately go into JSON (not a raw shell string), the validation correctly prevents heredoc breakout and other shenanigans.
• mcp_required_hosts("ado") correctly wires in the ADO domain allowlist when tools.azure-devops is enabled — so the network firewall gets updated automatically without user action.
• The skip logic (if name == ADO_MCP_SERVER_NAME && mcp_servers.contains_key(...)) correctly prevents duplicate MCPG entries when a user has both tools.azure-devops and mcp-servers.azure-devops, and the warning message is clear.
• Module rename src/tools/ → src/safeoutputs/ + new src/tools/ is a meaningful semantic distinction; the NON_MCP_SAFE_OUTPUT_KEYS being reduced to &[] is a correct consequence of moving memory out.
• Test coverage for new deserialization paths (CacheMemoryToolConfig, AzureDevOpsToolConfig) is thorough, including Enabled(false) and partial-config cases.

Generated by Rust PR Reviewer for issue #167 · ● 3.2M · ◷